-
Notifications
You must be signed in to change notification settings - Fork 22
🤖 feat: add local background bash process execution #749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
57f5c52 to
12287f3
Compare
This comment has been minimized.
This comment has been minimized.
a4d56f4 to
decd1a5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Adds ability to run long-running bash processes in the background without
blocking the AI conversation. Useful for builds, test suites, dev servers,
and other processes that take longer than typical tool timeouts.
Core components:
- CircularBuffer<T>: O(1) ring buffer for stdout/stderr (1000 lines each)
- BackgroundProcessManager: lifecycle management with spawn/list/terminate/cleanup
- Process IDs use 'bg-{8-hex-chars}' format for uniqueness
New tools:
- bash tool: enhanced with run_in_background parameter
- bash_background_read: check status and retrieve buffered output
- bash_background_list: discover processes after context loss
- bash_background_terminate: graceful shutdown (SIGTERM → SIGKILL)
Features:
- Automatic cleanup on workspace deletion
- Output buffers preserved after process exit
- Filtering support in read tool (tail, regex)
- Workspace isolation (processes scoped to workspace)
Test coverage: 84 tests across all components
decd1a5 to
78e19d3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Add supportsBackgroundExecution property to Runtime interface to detect runtime capability. SSH workspaces return clear error message instead of failing with ENOENT or running on wrong host. TODO comments added for future SSH support via nohup/setsid.
This comment was marked as resolved.
This comment was marked as resolved.
The onExit callback was unconditionally setting status to 'exited', overwriting the 'killed' status set by terminate(). Now guards with if (process.status === 'running') to preserve killed/failed states. Added test to verify killed status is preserved after termination.
This comment was marked as resolved.
This comment was marked as resolved.
When a process is killed by signal (SIGTERM, SIGKILL, etc.), Node's close event provides code=null and signal name. Previously we defaulted null to 0, making killed processes appear as successful exits. Now converts signals to Unix-conventional exit codes (128 + signal_number): - SIGKILL (9) → 137 - SIGTERM (15) → 143 - SIGINT (2) → 130 This allows bash_background_read/list to distinguish clean exits from forced terminations (via terminate() or external kill/OOM).
This comment was marked as resolved.
This comment was marked as resolved.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
… background execution - Add BackgroundExecutor interface and BackgroundHandle for abstracting process spawning - Create LocalBackgroundExecutor wrapping BashExecutionService for local processes - Add SSHBackgroundExecutor stub (returns 'not yet implemented' error) - BackgroundProcessManager now uses per-workspace executor registration - Remove pid from public API (runtime-specific detail) - Remove supportsBackgroundExecution from Runtime interface (executor error suffices) - Share single BashExecutionService instance across all local executors This refactor prepares for SSH background execution support by making the execution backend swappable per workspace.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
b139fe0 to
9b0c3af
Compare
…start - Add ensureExecutorRegistered() called from getOrCreateSession() - Add Config.getWorkspaceMetadataSync() for sync metadata lookup - Make registerExecutor() idempotent (no-op if already registered) Without this fix, background execution tools failed for all pre-existing workspaces after app restart with 'No executor registered' error.
9b0c3af to
2dd5e75
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Moves from pre-registration to lazy executor creation/caching: - Remove registerExecutor()/unregisterExecutor() from BackgroundProcessManager - spawn() now takes executor as first arg, caches per workspace - AIService creates executor alongside runtime in streamMessage() - Remove ensureExecutorRegistered() and getWorkspaceMetadataSync() - Remove 3 registration calls from IpcMain workspace creation paths This follows the existing pattern where runtime is created on-demand from metadata rather than pre-registered. Executors are still cached for SSH connection pooling. Net ~100 LoC removed.
Remove BackgroundExecutor abstraction layer - Runtime already handles local vs SSH execution differences, so background spawning belongs there. - Add spawnBackground() to Runtime interface with BackgroundHandle type - Implement LocalBackgroundHandle in LocalRuntime with event buffering - Stub SSHRuntime.spawnBackground() (not supported for SSH workspaces) - Simplify BackgroundProcessManager to call runtime directly - Update bash tool to pass runtime from ToolConfiguration - Remove aiService executor registration (no longer needed) - Delete backgroundExecutor.ts, localBackgroundExecutor.ts, sshBackgroundExecutor.ts ~300 LoC removed, cleaner separation of concerns.
- Only include bash_background_* tools when backgroundProcessManager available (fixes tools being exposed in CLI/debug paths where they can't work) - Extract LocalBackgroundHandle to separate file for better organization
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…cesses When a background process is killed by signal (SIGTERM, SIGKILL, etc.), Node's exit event provides code=null and signal name. The spawnBackground handler was defaulting null to 0, making killed processes appear as successful exits. - Add getExitCode() helper to convert signals to Unix-conventional exit codes (128 + signal_number): SIGTERM=143, SIGKILL=137, etc. - Add test verifying terminated processes report exit code >= 128 This restores behavior that was lost when bashExecutionService.ts was removed during the Runtime refactor.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
60d2540 to
af940fd
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Check both exitCode and signalCode when determining if process is alive - Fixes terminate() always sending SIGKILL after SIGTERM, even when process already stopped (signal-killed processes have signalCode set but exitCode null) - Also gate background tools for SSH runtimes (TODO: replace duck-type check with proper capability flag once SSH implements spawnBackground)
af940fd to
51dfcfd
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A single TextDecoder with stream:true buffers partial multibyte sequences. When stdout and stderr interleave, shared state corrupts output by combining bytes from different streams. Use independent decoders and flush on exit.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Adds terminateAll() to BackgroundProcessManager and hooks it into Electron's before-quit event, preventing orphaned processes when mux exits normally. Crash recovery (orphans from SIGKILL) remains out of scope.
Adds ability to run long-running bash processes in the background without blocking the AI conversation. Useful for builds, test suites, dev servers, and other processes that take longer than typical tool timeouts.
Core Components
bg-{8-hex-chars}format for uniquenessNew Tools
bash(enhanced)run_in_backgroundparameter to spawn without blockingbash_background_readbash_background_listbash_background_terminateFeatures
Limitations
nohup/setsid+ file-based output capture).Test Coverage
84 tests across all components (CircularBuffer, BackgroundProcessManager, bash tool, and all three background tools).
Generated with
mux